Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom archive mode #627

Merged
merged 76 commits into from
Jun 5, 2021
Merged

Conversation

altendky
Copy link
Collaborator

@altendky altendky commented May 22, 2021

Further building on #88.

Draft for:

  • Continued cleanup
  • Maybe presets
  • Make archive subcommand work as well
  • Wiki the fact that there are configuration versions
  • Update wiki for new archival configuration
  • The scripts presently take no command line parameters so the log lines for them are pretty useless
  • Logging of script output to catch errors
  • It is a bit silly to have to use quotes on the port like "12000", even if understandable as an environment variable key in the YAML
  • Trailing /-agnosticness?
  • set -evx for disk space script ends up outputting via stderr in the interactive log
  • Tidy up commented out code etc at least

jkbecker and others added 30 commits April 2, 2021 09:06
Co-authored-by: Kyle Altendorf <[email protected]>
Co-authored-by: Kyle Altendorf <[email protected]>
welp, that was straightforward

Co-authored-by: Kyle Altendorf <[email protected]>
Co-authored-by: Kyle Altendorf <[email protected]>
Copy link
Contributor

@rafaelsteil rafaelsteil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. It doesn't seem that src/plotman/resources/target_definitions.yaml is being copied to the config dir when running plotman config generate. I'd expect it to be the case, since users will potentially need / want to set custom values

  2. Running plotman archive from a fresh install crashes with the following exception, as arch_cfg is not being passed:

...starting archive loop
Traceback (most recent call last):
  File "/usr/local/bin/plotman", line 33, in <module>
    sys.exit(load_entry_point('plotman', 'console_scripts', 'plotman')())
  File "/Users/rafaelsteil/Desktop/custom-archive/plotman/src/plotman/plotman.py", line 193, in main
    archiving_status, log_messages = archive.spawn_archive_process(cfg.directories, jobs)
TypeError: spawn_archive_process() missing 1 required positional argument: 'all_jobs'

src/plotman/plotman.py Outdated Show resolved Hide resolved
src/plotman/configuration.py Outdated Show resolved Hide resolved
Comment on lines +50 to +55
except FileNotFoundError as e:
message = (
f'Unable to open log file. Verify that the directory exists'
f' and has proper write permissions: {log_file_path!r}'
)
raise Exception(message) from e
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be prudent to check it on startup rather than here. I added a snipped in another comment.

Comment on lines +183 to +184
elif args.cmd == 'archive':
print('...starting archive loop')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a check to make sure we'll be able to actually write the logs before even starting: (related to another comment I made)

if not path.exists(cfg.directories.log) or not os.access(cfg.directories.log, os.W_OK):
    print(f"Error: Directory '{cfg.directories.log}' doesn't exist or isn't writable")
    sys.exit(1)

src/plotman/archive.py Outdated Show resolved Hide resolved
src/plotman/archive.py Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@altendky altendky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of good catches here, thanks. Here are responses to some but not all of them.

target_definitions.yaml is specifically for the builtin presets. Users can write their own target definitions in the regular plotman configuration at archiving: target_definitions: my_target: (or whatever name they choose). So, I'll lump this into the 'needs documentation' task, unless I have missed your point. Probably a commented out example in the plotman.yaml as well, I guess?

src/plotman/plotman.py Outdated Show resolved Hide resolved
src/plotman/archive.py Outdated Show resolved Hide resolved
src/plotman/archive.py Outdated Show resolved Hide resolved
src/plotman/configuration.py Outdated Show resolved Hide resolved
@altendky altendky marked this pull request as ready for review June 5, 2021 23:33
@altendky altendky merged commit ec01e62 into ericaltendorf:development Jun 5, 2021
@altendky altendky deleted the custom_archive branch June 5, 2021 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants